-
Notifications
You must be signed in to change notification settings - Fork 15.2k
release/21.x: [DebugInfo] When referencing structured bindings use the reference's location, not the binding's declaration's location (#153637) #156664
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@Michael137 What do you think about merging this PR to the release branch? |
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-clang Author: None (llvmbot) ChangesBackport 665e875 Requested by: @Michael137 Full diff: https://github.com/llvm/llvm-project/pull/156664.diff 4 Files Affected:
diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp
index c8c3d6b20c496..5344a9710d642 100644
--- a/clang/lib/CodeGen/CGCall.cpp
+++ b/clang/lib/CodeGen/CGCall.cpp
@@ -4787,19 +4787,6 @@ struct DestroyUnpassedArg final : EHScopeStack::Cleanup {
}
};
-struct DisableDebugLocationUpdates {
- CodeGenFunction &CGF;
- bool disabledDebugInfo;
- DisableDebugLocationUpdates(CodeGenFunction &CGF, const Expr *E) : CGF(CGF) {
- if ((disabledDebugInfo = isa<CXXDefaultArgExpr>(E) && CGF.getDebugInfo()))
- CGF.disableDebugInfo();
- }
- ~DisableDebugLocationUpdates() {
- if (disabledDebugInfo)
- CGF.enableDebugInfo();
- }
-};
-
} // end anonymous namespace
RValue CallArg::getRValue(CodeGenFunction &CGF) const {
@@ -4836,7 +4823,9 @@ void CodeGenFunction::EmitWritebacks(const CallArgList &args) {
void CodeGenFunction::EmitCallArg(CallArgList &args, const Expr *E,
QualType type) {
- DisableDebugLocationUpdates Dis(*this, E);
+ std::optional<DisableDebugLocationUpdates> Dis;
+ if (isa<CXXDefaultArgExpr>(E))
+ Dis.emplace(*this);
if (const ObjCIndirectCopyRestoreExpr *CRE =
dyn_cast<ObjCIndirectCopyRestoreExpr>(E)) {
assert(getLangOpts().ObjCAutoRefCount);
@@ -6229,3 +6218,12 @@ RValue CodeGenFunction::EmitVAArg(VAArgExpr *VE, Address &VAListAddr,
return CGM.getABIInfo().EmitMSVAArg(*this, VAListAddr, Ty, Slot);
return CGM.getABIInfo().EmitVAArg(*this, VAListAddr, Ty, Slot);
}
+
+DisableDebugLocationUpdates::DisableDebugLocationUpdates(CodeGenFunction &CGF)
+ : CGF(CGF) {
+ CGF.disableDebugInfo();
+}
+
+DisableDebugLocationUpdates::~DisableDebugLocationUpdates() {
+ CGF.enableDebugInfo();
+}
diff --git a/clang/lib/CodeGen/CGCall.h b/clang/lib/CodeGen/CGCall.h
index 0b4e3f9cb0365..17b2ce558f711 100644
--- a/clang/lib/CodeGen/CGCall.h
+++ b/clang/lib/CodeGen/CGCall.h
@@ -457,6 +457,12 @@ inline FnInfoOpts &operator&=(FnInfoOpts &A, FnInfoOpts B) {
return A;
}
+struct DisableDebugLocationUpdates {
+ CodeGenFunction &CGF;
+ DisableDebugLocationUpdates(CodeGenFunction &CGF);
+ ~DisableDebugLocationUpdates();
+};
+
} // end namespace CodeGen
} // end namespace clang
diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index 85c768807572f..b5debd93b0f61 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -3314,7 +3314,14 @@ LValue CodeGenFunction::EmitDeclRefLValue(const DeclRefExpr *E) {
auto *FD = LambdaCaptureFields.lookup(BD);
return EmitCapturedFieldLValue(*this, FD, CXXABIThisValue);
}
- return EmitLValue(BD->getBinding());
+ // Suppress debug location updates when visiting the binding, since the
+ // binding may emit instructions that would otherwise be associated with the
+ // binding itself, rather than the expression referencing the binding. (this
+ // leads to jumpy debug stepping behavior where the location/debugger jump
+ // back to the binding declaration, then back to the expression referencing
+ // the binding)
+ DisableDebugLocationUpdates D(*this);
+ return EmitLValue(BD->getBinding(), NotKnownNonNull);
}
// We can form DeclRefExprs naming GUID declarations when reconstituting
diff --git a/clang/test/CodeGenCXX/debug-info-structured-binding.cpp b/clang/test/CodeGenCXX/debug-info-structured-binding.cpp
index 5fbd54c16382c..4a4a4d8bdfaad 100644
--- a/clang/test/CodeGenCXX/debug-info-structured-binding.cpp
+++ b/clang/test/CodeGenCXX/debug-info-structured-binding.cpp
@@ -7,6 +7,12 @@
// CHECK: #dbg_declare(ptr %{{[0-9]+}}, ![[VAR_4:[0-9]+]], !DIExpression(DW_OP_deref, DW_OP_plus_uconst, 4),
// CHECK: #dbg_declare(ptr %z1, ![[VAR_5:[0-9]+]], !DIExpression()
// CHECK: #dbg_declare(ptr %z2, ![[VAR_6:[0-9]+]], !DIExpression()
+// CHECK: getelementptr inbounds nuw %struct.A, ptr {{.*}}, i32 0, i32 1, !dbg ![[Y1_DEBUG_LOC:[0-9]+]]
+// CHECK: getelementptr inbounds nuw %struct.A, ptr {{.*}}, i32 0, i32 1, !dbg ![[Y2_DEBUG_LOC:[0-9]+]]
+// CHECK: load ptr, ptr %z2, {{.*}}!dbg ![[Z2_DEBUG_LOC:[0-9]+]]
+// CHECK: getelementptr inbounds [2 x i32], ptr {{.*}}, i64 0, i64 1, !dbg ![[A2_DEBUG_LOC:[0-9]+]]
+// CHECK: getelementptr inbounds nuw { i32, i32 }, ptr {{.*}}, i32 0, i32 1, !dbg ![[C2_DEBUG_LOC:[0-9]+]]
+// CHECK: extractelement <2 x i32> {{.*}}, i32 1, !dbg ![[V2_DEBUG_LOC:[0-9]+]]
// CHECK: ![[VAR_0]] = !DILocalVariable(name: "a"
// CHECK: ![[VAR_1]] = !DILocalVariable(name: "x1", scope: !{{[0-9]+}}, file: !{{[0-9]+}}, line: {{[0-9]+}}, type: !{{[0-9]+}})
// CHECK: ![[VAR_2]] = !DILocalVariable(name: "y1", scope: !{{[0-9]+}}, file: !{{[0-9]+}}, line: {{[0-9]+}}, type: !{{[0-9]+}})
@@ -46,5 +52,41 @@ int f() {
auto [x1, y1] = a;
auto &[x2, y2] = a;
auto [z1, z2] = B{1, 2};
- return x1 + y1 + x2 + y2 + z1 + z2;
+ int array[2] = {3, 4};
+ auto &[a1, a2] = array;
+ _Complex int cmplx = {1, 2};
+ auto &[c1, c2] = cmplx;
+ int vctr __attribute__ ((vector_size (sizeof(int)*2)))= {1, 2};
+ auto &[v1, v2] = vctr;
+ return //
+ x1 //
+ + //
+// CHECK: ![[Y1_DEBUG_LOC]] = !DILocation(line: [[@LINE+1]]
+ y1 //
+ + //
+ x2 //
+ + //
+// CHECK: ![[Y2_DEBUG_LOC]] = !DILocation(line: [[@LINE+1]]
+ y2 //
+ + //
+ z1 //
+ + //
+// CHECK: ![[Z2_DEBUG_LOC]] = !DILocation(line: [[@LINE+1]]
+ z2 //
+ + //
+ a1 //
+ + //
+// CHECK: ![[A2_DEBUG_LOC]] = !DILocation(line: [[@LINE+1]]
+ a2 //
+ + //
+ c1 //
+ + //
+// CHECK: ![[C2_DEBUG_LOC]] = !DILocation(line: [[@LINE+1]]
+ c2 //
+ + //
+ v1 //
+ + //
+// CHECK: ![[V2_DEBUG_LOC]] = !DILocation(line: [[@LINE+1]]
+ v2 //
+ ;
}
|
@Michael137 (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR. |
…location, not the binding's declaration's location (llvm#153637) For structured bindings that use custom `get` specializations, the resulting LLVM IR ascribes the load of the result of `get` to the binding's declaration, rather than the place where the binding is referenced - this caused awkward sequencing in the debug info where, when stepping through the code you'd step back to the binding declaration every time there was a reference to the binding. To fix that - when we cross into IRGening a binding - suppress the debug info location of that subexpression. I don't represent this as a great bit of API design - certainly open to ideas, but putting it out here as a place to start. It's /possible/ this is an incomplete fix, even - if the binding decl had other subexpressions, those would still get their location applied & it'd likely be wrong. So maybe that's a direction to go with to productionize this - add a new location scoped device that suppresses any overriding - this might be more robust. How do people feel about that? (cherry picked from commit 665e875)
Do we also need to backport 5b4819e ? I'm seeing the test from this commit fail on our i686 builders. |
Ok, look like the test name changed in main since the release branch was created, so we may need this fix after all. |
@tstellar I am guessing we don't have a i686 pre-commit bot then since this wasn't caught before we merged this. @dwblaikie can you look into a fix for the release branch so we can fix it for next release? |
Yeah, sorry for the delay - sent #159209 - hopefully that's suitable, I haven't tried making a manual PR for the release branch before. |
Backport 665e875
Requested by: @Michael137